-
Notifications
You must be signed in to change notification settings - Fork 1
Simple allocator #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments for your consideration, but looking great!
function isValidSignature(bytes32 hash, bytes calldata) external view returns (bytes4 magicValue) { | ||
// The hash is the digest of the compact | ||
bytes32 tokenHash = _sponsor[hash]; | ||
if (tokenHash == bytes32(0) || _claim[tokenHash] <= block.timestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also confirm that hasConsumedAllocatorNonce is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we need to do that, since we can confidently rely on the Compact contract to have checked the nonce consumption prior to the signature verification... Or did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah yeah that check does happen prior; so i even think this check would always fail 🙃
} | ||
|
||
/// @dev copied from IdLib.sol | ||
function _resetPeriodToSeconds(ResetPeriod resetPeriod_) internal pure returns (uint256 duration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to actually import IdLib and use it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that... Kind of felt a little overkill for these three lines of code. Also, I felt like it would make the contract easier accessible to someone who has not previously interacted with the Compact before. But definitely open to a different opinion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main reason i'd advise it is so that any upstream changes get incorporated; but agreed it's not strictly necessary!
@@ -2,14 +2,14 @@ | |||
|
|||
pragma solidity ^0.8.27; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have tests that use the actual contract and not just this mock? should probably use the proper contract in at least some cases if we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, thats why I ended up using the proper Compact contract in the ERC7683Allocator tests. The reason why I did not use it in this allocator "template" is just so the compile times stay fast for devs building on top of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it!
Updates to support reversed dutch auction
Erc7683 allocator
commit 66fd55f Author: mgretzke <[email protected]> Date: Fri Aug 1 15:01:56 2025 +0200 cleanup commit fb8f8e5 Merge: 324782a 0ac955d Author: Mark Gretzke <[email protected]> Date: Tue Apr 1 14:11:04 2025 +0200 Merge pull request #5 from Uniswap/SimpleAllocator Simple allocator commit 0ac955d Merge: a2fb41b 273e632 Author: Mark Gretzke <[email protected]> Date: Tue Apr 1 09:45:16 2025 +0200 Merge pull request #6 from Uniswap/ERC7683Allocator Erc7683 allocator commit 273e632 Merge: 4bed9de 10162ae Author: 0age <[email protected]> Date: Thu Mar 27 12:36:19 2025 -0400 Merge pull request #7 from Uniswap/mandate-reverse-dutch-auction Updates to support reversed dutch auction commit a2fb41b Author: vimageDE <[email protected]> Date: Thu Mar 6 18:51:00 2025 +0100 enable relayed locks in child contracts commit 32dafd3 Author: vimageDE <[email protected]> Date: Wed Mar 5 11:34:37 2025 +0100 use this.attest.selector commit f91a8cc Author: vimageDE <[email protected]> Date: Wed Mar 5 11:05:53 2025 +0100 Removed sponsor == operator requirement
Pull Request
Description
A simple, fully decentralized allocator that allows for a single claim per token. This means the contract will lock down all tokens of a sponsor for an id for a single claim, so it is not possible to start multiple claims for the same sponsor and id at the same time.
The contract does keep track of the amount of unlocked tokens and will faithfully attest for a transfer of those, even during an ongoing claim. The contract is a good starting point when learning about allocators and it is kept very simple on purpose, to learn about the concept of an allocator or use this contract as a template. To be used in production, the contract would require the ability to work with witness data, since a real cross chain swap will always require additional witness data.
How Has This Been Tested?
The contract is still in development and neither audited, nor has it been fully tested.